Skip to content
This repository has been archived by the owner on Nov 22, 2022. It is now read-only.

fix(config): Use ioutil.WriteFile on Windows #212

Merged
merged 1 commit into from
Sep 7, 2020

Conversation

andhe
Copy link
Contributor

@andhe andhe commented Sep 7, 2020

As reported renameio doesn't work on Windows and apparently there's
no way to do atomic file replace on Windows.

We thus create a helper function in a separate file that uses renameio
but say it should not be built on windows, then hook in a windows-
specific implementation of that function which uses ioutil.WriteFile
which is not safe but gets the job done.

While at it also tighten up the permissions on the config file as
it might contain sensitive information (like the token).

Fixes #210

As reported renameio doesn't work on Windows and apparently there's
no way to do atomic file replace on Windows.

We thus create a helper function in a separate file that uses renameio
but say it should not be built on windows, then hook in a windows-
specific implementation of that function which uses ioutil.WriteFile
which is not safe but gets the job done.

While at it also tighten up the permissions on the config file as
it might contain sensitive information (like the token).

Fixes profclems#210
@pull-assistant
Copy link

pull-assistant bot commented Sep 7, 2020

Score: 1.00

Best reviewed: commit by commit


Optimal code review plan

     fix(config): Use ioutil.WriteFile on Windows

Powered by Pull Assistant. Last update 2b4df29 ... 2b4df29. Read the comment docs.

@andhe
Copy link
Contributor Author

andhe commented Sep 7, 2020

Note that this has not actually been tested on windows (except GOOS=windows cross-build).

@andhe
Copy link
Contributor Author

andhe commented Sep 7, 2020

I also have no idea how to make renameio in go.sum not apply on windows. I guess that means people building on Windows will still have to download renameio even if it will not be used in the build?

Copy link
Owner

@profclems profclems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andhe!
I will build and test on windows and merge if it passes.

@profclems profclems merged commit bdfd234 into profclems:trunk Sep 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to update config file on windows
2 participants